Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move EventLog tests to not run on CI #32501

Merged
merged 4 commits into from
Feb 18, 2020
Merged

Move EventLog tests to not run on CI #32501

merged 4 commits into from
Feb 18, 2020

Conversation

ViktorHofer
Copy link
Member

We are hitting Win32 API issues when calling specific EventLog features.
As the risk of changes in EventLog is low we decided to turn off some
tests in CI and run them manually before a release.

#32403 (comment) for discussion.

@ViktorHofer
Move EventLog tests to not run on CI
db36b2a 
We are hitting Win32 API issues when calling specific EventLog features.
As the risk of changes in EventLog is low we decided to turn off some
tests in CI and run them manually before a release.
@ViktorHofer ViktorHofer self-assigned this Feb 18, 2020
This was referenced Feb 18, 2020
Closed
Closed
@danmoseley
Copy link
Member

@Anipik we should add a step as part of shipping 5.0 previews - we should run IgnoreForCI tests locally. At the moment, just Windows 10 should be fine. Perhaps this is something you can do after you create each preview branch?

The goal here is a simple check to make sure we didn't completely break something - EventLog in this case.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 18, 2020 •
edited
Loading

@Anipik we should add a step as part of shipping 5.0 previews - we should run IgnoreForCI tests locally. At the moment, just Windows 10 should be fine. Perhaps this is something you can do after you create each preview branch?

Ideally we would just run all tests locally as IgnoreForCI tests aren't excluded locally and it would give us more coverage (local vs CI).

@davidsh davidsh mentioned this pull request Feb 18, 2020
Open
@danmoseley
Copy link
Member

Ideally we would just run all tests locally as IgnoreForCI tests aren't excluded locally and it would give us more coverage (local vs CI).

Oh! Of course. We might not get good local coverage of Outerloop, or Linux. But not relevant here: we should be getting coverage for free then. Nice.

@ViktorHofer ViktorHofer merged commit 48a222c into dotnet:master Feb 18, 2020
safern
safern reviewed Feb 18, 2020
View reviewed changes
...libraries/System.Diagnostics.EventLog/tests/System/Diagnostics/Reader/EventLogRecordTests.cs
@@ -29,6 +30,7 @@ public void GetPropertyValues()
}

[ActiveIssue("https://github.com/dotnet/corefx/issues/34547")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this ActiveIssue left in purpose?

@Anipik
Copy link
Contributor

Anipik commented Feb 19, 2020

@Anipik we should add a step as part of shipping 5.0 previews - we should run IgnoreForCI tests locally. At the moment, just Windows 10 should be fine. Perhaps this is something you can do after you create each preview branch?

I will start doing that for previews

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@danmoseley danmoseley danmoseley approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@ViktorHofer ViktorHofer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ViktorHofer @danmoseley @Anipik @safern